core: unify lua and :set option handling#1349
Open
rnpnr wants to merge 1 commit intomartanne:masterfrom
Open
Conversation
Collaborator
Author
|
With respect to having a @fischerling I know you register a I do believe the additional function pointer is valuable on the C side because it would be required for a possible C plugin interaction (the rest of vis is already kind of set up for this even though its not implemented). |
rnpnr
force-pushed
the
lua_option_merge
branch
3 times, most recently
from
391e8fe to
23c5b38
Compare
The only thing that needs to be different about these paths is how the value that will be assigned to option is obtained. When the value comes from lua we can ask lua directly for the expected type but when it comes from :set we need to parse it ourselves. Besides that lookup of the option, and setting of option should not be using different codepaths. By combining them we no longer need worry about modifying multiple places in the code to add an option or update its handling. The addition of a vis_option_get() helper should also facilitate easy building of a full table of option values to pass to lua but that is not implemented here. Furthermore the C-API was updated to add a VisOptionGetFunction pointer to the option registration which would allow custom options to be included in the table but since addition to lua would require breaking the existing API it needs to be further discussed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The only thing that needs to be different about these paths is how the value that will be assigned to option is obtained. When the value comes from lua we can ask lua directly for the expected type but when it comes from :set we need to parse it ourselves.
Besides that, lookup of the option, and setting of the option should not be using different codepaths. By combining them we no longer need worry about modifying multiple places in the code to add an option or update its handling.
The addition of a vis_option_get() helper should also facilitate easy building of a full table of option values to pass to lua but that is not implemented here. Furthermore the C-API was updated to add a VisOptionGetFunction pointer to the option registration which would allow custom options to be included in the table but since addition to lua would require breaking the existing API it needs to be further discussed.